Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the ldap certificate value properly. #139

Closed
wants to merge 1 commit into from

Conversation

sandyg1
Copy link

@sandyg1 sandyg1 commented Jan 30, 2019

Set the ldap certificate value properly. Also added a few comments to clarify the group attributes.

Set the ldap certificate value properly. Also added a few comments to clarify the group attributes.
@sandyg1
Copy link
Author

sandyg1 commented Jan 30, 2019

Added DCO

@@ -11,7 +11,8 @@
# Skip certificate verification. Default: false
insecure_skip_verify: ((ldap_insecure_skip_verify))
# The CA certificate for the LDAP auth provider’s endpoints.
ca_cert: ((ldap_ca_cert))
ca_cert:
certificate: ((ldap_ca_cert))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this is actually proper - it seems like the current ops file expects ldap_ca_cert to be a certificate typed variable

@brightzheng100 is that right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brightzheng100 @vito Then you cannot read the certificate from a file, rather you would have to set the variable in a file and interpolate it. Both work but I believe reading the certificate from a file is a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you @sandyg1.
Based on what I've tested here, it's better to make it more specific, even either way works.

@brightzheng100
Copy link
Contributor

This is an interesting thing while working with certificate.

For demonstration purposes, I've tested out with 3 scenarios:

Option 1

In ldap.yml -- asuming we keep current ops file as is:

ca_cert: ((ldap_ca_cert))

Deployment:

$ credhub set -n /bosh-lite/concourse4/ldap_ca_cert -t certificate --certificate ~/workspaces/CA-CERTS/certs/ca.crt.pem
$ bosh deploy -d concourse4 concourse.yml \
  ...
  -v ldap_ca_cert="((ldap_ca_cert))" \
  ...

It works.

Option 2

In ldap.yml, if we accept the @vito's suggestion:

ca_cert:
  certificate: ((ldap_ca_cert))

Deployment:

$ bosh deploy -d concourse4 concourse.yml \
  ...
  --var-file ldap_ca_cert=~/workspaces/CA-CERTS/certs/ca.crt.pem \
  ...

It works too.

Option 3

ldap.yml:

ca_cert:
  certificate: ((ldap_ca_cert))

Deployment:

$ credhub set -n /bosh-lite/concourse4/ldap_ca_cert -t certificate --certificate ~/workspaces/CA-CERTS/certs/ca.crt.pem
$ bosh deploy -d concourse4 concourse.yml \
  ...
  -v ldap_ca_cert="((ldap_ca_cert.certificate))" \
  ...

It also works.

Frankly, I'd say the way @vito highlighted is more precise.

Meanwhile, I'll reach out further to some other teams to get back the "best practice".

More feedback?

@vito
Copy link
Member

vito commented Mar 10, 2019

@brightzheng100 Thanks for the investigation!

Come to think of it, I think -v and --var-file also let you set individual fields, so even if you have ca_cert: ((ldap_ca_cert)), I think you can do this:

$ bosh deploy -d concourse4 concourse.yml \
  ...
  --var-file ldap_ca_cert.certificate=~/workspaces/CA-CERTS/certs/ca.crt.pem \
  ...

At least, I remember that working with -v. I haven't tried with --var-file.

@sandyg1 Does the above work for you?

@mjenk664
Copy link

Hi @sandyg1 @vito,

I am working with a customer right now who is looking to set up Concourse w/ LDAP Auth. We ran into this same challenges as Vito above. Having only the ca_cert: ((ldap_ca_cert)) property defined fails to actually add in the certificate.

I came across this PR and we tested and validated that this indeed is the culprit. After adding the certificate: field to the ca_cert: property, things are working properly :)

It's been a while since this PR has received any activity, so I thought I'd help others out. It'd be great if this could get merged in for those who want to configure ldap in the future.

@vito
Copy link
Member

vito commented May 21, 2019

@marijenk Thanks for the update. I feel like this is a documentation issue, though, and we shouldn't merge this PR. We've actually received a couple other similar PRs in the past which have been closed with the same reasoning:

These ops files expect typed variables to be specified for these properties, as the BOSH property specifies it as type: certificate.

This allows the value to be specified using a generated var or provided by a certificate-typed credential in CredHub. If we hardcode certificate: ((foo)) they can only be specified as string values, which seems like a bit of a shame. Technically these fields are typically satisfied by a static value, making the generated vars use case a bit moot, but it still seems like a good practice to use typed values when possible if you're e.g. storing credentials in CredHub.

This has come up three times now in the past couple of weeks, though, so I think something should change. It feels like we're using the BOSH abstractions correctly, so I guess we just need to update documentation...somewhere. I'm not sure where. 😕 Is there anywhere you would have looked to see that kind of thing?

Closing; I've opened an issue for updating documentation here: #163

Thanks!

@vito vito closed this May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants